Skip to content

Conversation

@rezzmah
Copy link

@rezzmah rezzmah commented Jun 25, 2025

This PR merges changes from upstream paradigmxyz#16927

rose2221 and others added 30 commits June 9, 2025 00:11
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
This fixes critical backward compatibility issues in the base fee centralization:

- Update EthChainSpec::next_block_base_fee to accept target_timestamp parameter
- Use correct timestamps for base fee parameter fetching:
  - Consensus validation: header.timestamp() (target block timestamp)
  - EVM: attributes.timestamp (target block timestamp)
  - RPC fee history: parent timestamp (for unknown future block)
  - Transaction pool: current tip timestamp (for unknown future block)
  - E2E tests: current block timestamp

The original implementation was using parent.timestamp() which could cause
consensus failures when base fee parameters change between blocks.
- Remove duplicate trait bounds in EthChainSpec
- Add documentation for base fee test module
- Remove unnecessary cast for INITIAL_BASE_FEE
- Move base fee test to existing test module instead of separate file
- Remove unnecessary trait bound from EthChainSpec::Header
- Maintain backward compatibility by keeping original trait signature

This reduces the diff size and follows reth's testing conventions.
- Remove leftover commented trait bounds in RPC core
- Fix circular test assertions in optimism base fee tests
- Restore proper test logic that compares against direct calculation

This ensures tests actually validate the centralized calculation works correctly.
The Self: Sized constraint is required to make EthChainSpec dyn-safe,
as it is used as a trait object in the ExEx system. Removing this
constraint breaks compilation due to the generic method.
Replace complex block fetching with direct header-based calculation
using minimal Header struct. This eliminates unnecessary database
lookups and error handling while maintaining the same functionality.

- Add next_block_base_fee method to FeeHistoryEntry
- Remove redundant block fetching in fee history calculation
- Clean up unused imports
Replace manual base fee parameter lookup with direct chainspec method
call, making the code cleaner and more consistent with the centralized
base fee calculation approach.
Store chain_spec in variable to avoid multiple provider calls in
the same function scope, improving performance by reducing redundant
method calls.
- Remove verbose comments that don't add value
- Fix line length formatting to meet Reth standards
- Apply consistent code style via rustfmt
Add clear warning to implementers that some callers (like fee_history)
pass minimal headers with only gas_used, gas_limit, base_fee_per_gas,
and timestamp populated. This prevents implementers from relying on
additional header fields that may not be available.
rezzmah added 19 commits June 19, 2025 09:55
Update test assertions to use chain_spec.next_block_base_fee instead of
manual header.next_block_base_fee calls for consistency with the
centralized approach.
Fix clippy doc_markdown warnings by adding backticks around code
items in the caution documentation for next_block_base_fee method.
Link to AlloyBlockHeader::next_block_base_fee instead of duplicating
the implementation details in the trait documentation.
Instead of taking a full header, next_block_base_fee now takes:
- parent_gas_used: u64
- parent_gas_limit: u64
- parent_base_fee_per_gas: u64
- timestamp: u64

This simplifies the API and avoids creating minimal headers just to
pass gas fields, as noted in PR feedback. Removes unnecessary helper
method from FeeHistoryEntry since it's no longer needed.
- Use proper error handling instead of unwrap_or_default for base fee
- Remove unused Header import from fee_history.rs
Explains that the BaseFeeMissing error for parent blocks won't occur
in practice since previous blocks are validated to have base fees.
In e2e tests, we expect base_fee_per_gas to always be present,
so using unwrap() provides better error reporting if it's missing.
Revert back to using the optimism-specific next_block_base_fee function
which handles Holocene hardfork logic and proper error propagation with ?,
matching the original behavior before the trait signature changes.
- Move import to maintain alphabetical order
- Add blank line for better readability
Replace broken AlloyBlockHeader::next_block_base_fee reference
with calc_next_block_base_fee which is the actual function used
in the implementation.
Add parent_timestamp parameter to the next_block_base_fee method signature
to enable time-based validations and hardfork transition logic that may
require both parent and target block timestamps.

Changes:
- Updated EthChainSpec trait method signature
- Updated all callers to provide parent timestamp
- Maintained backward compatibility of calculation logic
- Parent timestamp now available for future chain-specific implementations

The parent_timestamp parameter is currently unused in the base implementation
but provides the foundation for more sophisticated base fee calculations.
Centralizes base fee calculation through EthChainSpec trait while following
alloy-consensus pattern of returning Option<u64> instead of u64.

Changes:
- Update EthChainSpec::next_block_base_fee to return Option<u64>
- Add Self: Sized constraint to maintain trait object compatibility
- Use header parameter approach instead of individual fields
- Update all callers to handle Option return with unwrap_or_default()
- Maintain backward compatibility for Optimism-specific logic
- Preserve original variable patterns to minimize diff

The Option return type indicates absence of EIP-1559 support (None = no base fee),
matching the pattern used in alloy-consensus crate.
…kups

This change implements a performance optimization suggested in PR paradigmxyz#16927 by storing
complete Header objects in FeeHistoryEntry instead of individual fields. This eliminates
the need for additional database header lookups when calculating next block base fees.

Key changes:
- Modified FeeHistoryEntry to store full Header instead of individual gas fields
- Updated FeeHistoryEntry::new() to accept any Block with BlockHeader (generic)
- Updated fee history construction to use stored headers directly
- Eliminated database lookup in fee calculation path (line 157-160 in fee.rs)
- Followed Reth patterns for direct field access over accessor methods

Performance impact:
- Eliminates one database query per fee history request
- Increases memory usage by ~720KB for typical cache (acceptable trade-off)
- Removes potential failure point from missing headers in database

Technical implementation:
- Single FeeHistoryEntry::new() method now handles all header types via BlockHeader trait
- Manual Header construction ensures compatibility across Ethereum, Optimism, and other chains
- Maintains full backward compatibility with existing API
Make both FeeHistoryCache<H> and FeeHistoryEntry<H> generic over header type H
to support different blockchain header implementations while maintaining
backward compatibility through default type parameters.

This enables alternative blockchain implementations (like Optimism, Polygon, BSC)
to use their own header types with custom fields while reusing the same fee
history infrastructure. The changes maintain type safety by ensuring cache
and entry types are compatible only when using the same header type.

Changes:
- FeeHistoryCache<H = Header> with generic header support
- FeeHistoryEntry<H = Header> with improved generic constructor
- Updated all trait definitions and implementations to use ProviderHeader<Provider>
- Added proper type constraints in builders to ensure header type consistency
- Maintained backward compatibility through default type parameters
@rezzmah rezzmah closed this Jun 26, 2025
@rezzmah
Copy link
Author

rezzmah commented Jun 26, 2025

Has been merged upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants